@devnexen @nielsdos I reached out to @nikic regarding the four failed tests.

Update: Fixed! The tests were failing because:

1. Empty strings weren't being handled as a special case (they should sort before numbers)
2. The transitive_compare_mode flag wasn't initialized (caused issues on Windows ZTS)

Hi @jmarble. Sorry for the delayed response. One immediate concern is that this introduces an obvious inconsistency for key and value sorting. These two should remain consistent.

$units = ['5', '3A'];
sort($units);
var_dump($units);
// array(2) {
//   [0]=>
//   string(1) "5"
//   [1]=>
//   string(2) "3A"
// }

$units = ['5' => 1, '3A' => 1];
ksort($units);
var_dump($units);
// array(2) {
//   ["3A"]=>
//   int(1)
//   [5]=>
//   int(1)
// }

There's an obvious performance impact, given how common comparisons are (+0.4% instruction count as suggested by the Symfony Demo benchmark). Whether there's a true real-world impact remains to be seen.

This is related to GH-9882. Enums are also plagued by them not being properly handled in array_unique(), so I proposed to introduce List\unique() and Assoc\unique() functions. Before that, we have also discussed a ARRAY_UNIQUE_IDENTICAL flag much like in your original idea, which was disliked by internals. Sadly, there was a lot of bikeshedding regarding naming and namespaces, so the proposal died down. Regardless, this does not resolve your problem for sort() (which isn't a concern for enums).

I also wonder how this relates to @Girgias' efforts to make comparisons in PHP transitive. Maybe she can comment on that.

Hi @iluuu1994 thank you for taking the time to review this so carefully. Admittedly, I got a bit too focused on resolving the issue I had run into. I'll push a commit here in a moment to resolve the discrepancy.

There's an obvious performance impact, given how common comparisons are (+0.4% instruction count as suggested by the Symfony Demo benchmark). Whether there's a true real-world impact remains to be seen.

The CI benchmark was actually only +0.04%, so only 0.04ms on a hypothetical 100ms request scenario. I think this is acceptable for correctness. We could always try compensating with a performance improvement somewhere.

I saw your email thread to internals regarding ARRAY_UNIQUE_IDENTICAL. I also emailed internals regarding the need for a "strict" comparison. I ended up creating a quick POC for an ARRAY_UNIQUE_STRICT` flag. I considered a formal RFC but got busy with other things.

The CI benchmark was actually only +0.04%, so only 0.04ms

@jmarble You're right, sorry. I mistyped. It's not completely insignificant (performance improvements are very hard to come by nowadays), but unlikely to be a blocker if we otherwise decide this is the right behavior.

I do think it may be useful to consider consolidating comparison as a whole. Gina has done some work on that, as mentioned. Maybe we can solve this in a more consistent manner. If not, I don't object to your solution.

The array_unique case specifically may be better served with a case that doesn't rely on sorting. Some people have suggested introducing a proper Set class. However, this opens another can of worms because (in my very strong opinion) data types should have value type semantics, which isn't currently possible for classes.

@iluuu1994 looks like we got a pretty favorable run on this last benchmark? A swing from +0.04% to -0.19% and faster across the board? Could be noise, or just more proof of negligible impact on performance.

I do think it may be useful to consider consolidating comparison as a whole.

I agree. Admittedly, I took the path of least resistance. I felt there was enough precedent for an EG flag.

The array_unique case specifically may be better served with a case that doesn't rely on sorting. Some people have suggested introducing a proper Set class. However, this opens another can of worms because (in my very strong opinion) data types should have value type semantics, which isn't currently possible for classes.

Yes, a proper Set class would most certainly open a can of worms haha. Prior to settling on this EG flag, I made some attempts to resolve the issue directly within array_unique with hash maps, but I found too many edge cases for my appetite, and I eventually realized that SORT_REGULAR was fundamentally flawed without transitive comparison.

For context, I had started thinking about what comparison semantics PHP should have and some of it is written down in one of my draft RFC: https://github.com/Girgias/php-rfcs/blob/master/comparison-equality-semantics.md

However, not that comparison operators cannot be transitive, even if you just include numeric values due to NaN never being equal to itself.

In general I believe that getting rid of "ordering" for strings with the comparison operators is a better approach and have people use strcmp() instead.

However, this doesn't really address SORT_REGULAR. And I don't really understand why you are changing the implementation of zend_compare() when seemingly you could create a new php_array_sort_regular() function that handles the transitivity issue for the limited cases needed and then defer to zend_compare()?

@Girgias thank you for sharing your RFC draft on equality semantics -- you make a compelling case!

To address your question: creating a standalone php_array_sort_regular() function would require duplicating not just the top-level comparison, but also zend_compare_arrays(), zend_objects_compare(), and their recursive call chains to handle nested structures (arrays containing arrays/objects with string properties).

Ultimately, I felt this path of least resistance was acceptable given the amount of refactoring that would be necessary to implement a proper alternative. Perhaps this solution might at least inspire someone who might be willing to make such a refactor?

In my recent commit, I found some opportunities to improve comparison performance. I noticed ZEND_THREEWAY_COMPARE() was more efficient than normal comparison, which I assume is due to better CPU optimization? I also implemented some "fast path" optimizations which provided a moderate performance gain over master (~30%) for integers with sort().

In real-world scenarios, I would expect no meaningful performance impact. However, the correctness bug this resolves is a demonstrable win.